codec, table: make new collation setting explicit in encoding (#69566)#69684
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
@D3Hunter This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
📝 WalkthroughWalkthroughThis PR makes collation handling explicit rather than relying on a global flag read at encode time. A ChangesCollation-aware encoder plumbing and lightweight bootstrap domain
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant InitGlobalVarFromSystemDB
participant domainMap
participant Domain
participant issyncer.Syncer
participant issyncer.Loader
InitGlobalVarFromSystemDB->>domainMap: GetOrCreateWithFilter(store, systemDBFilter)
domainMap->>Domain: NewDomainWithEtcdClient(schemaFilter)
Domain->>issyncer.Syncer: New(..., filter)
issyncer.Syncer->>issyncer.Loader: newLoader(..., filter)
issyncer.Loader-->>issyncer.Loader: SkipLoadDiff / SkipLoadSchema
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
pkg/table/tblctx/buffers.go (1)
90-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent encoder threading:
EncodeBinlogRowDatastill reads the global setting directly.
WriteMemBufferEncodednow takes an explicitenc codec.Encoderfrom the caller (line 56), butEncodeBinlogRowDatabuilds its own encoder inline viacollate.NewCollationEnabled()rather than accepting one as a parameter. This keeps an implicit global read inside the very layer this PR is trying to make explicit, and could diverge from the row's actual collation context if the caller's encoder differs from the global state at call time.Consider accepting
enc codec.Encoderas a parameter here too, mirroringWriteMemBufferEncoded, for consistency within this file.♻️ Proposed refactor
-func (b *EncodeRowBuffer) EncodeBinlogRowData(loc *time.Location, ec errctx.Context) ([]byte, error) { - enc := codec.NewEncoder(collate.NewCollationEnabled()) - value, err := tablecodec.EncodeOldRow(enc, loc, b.row, b.colIDs, nil, nil) +func (b *EncodeRowBuffer) EncodeBinlogRowData(enc codec.Encoder, loc *time.Location, ec errctx.Context) ([]byte, error) { + value, err := tablecodec.EncodeOldRow(enc, loc, b.row, b.colIDs, nil, nil)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/table/tblctx/buffers.go` around lines 90 - 97, EncodeBinlogRowData still creates its own encoder with collate.NewCollationEnabled(), which leaves an implicit global dependency and can disagree with the caller’s collation context. Update EncodeBinlogRowData to accept a codec.Encoder parameter, and thread that encoder through to tablecodec.EncodeOldRow instead of constructing it locally, matching the explicit encoder flow used by WriteMemBufferEncoded and the EncodeRowBuffer type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/meta/model/table.go`:
- Around line 584-596: Resolve the merge conflict in table.go by removing the
Git conflict markers and keeping the incoming GetIdxChangingFieldType helper.
Ensure the table package compiles by deleting the leftover <<<<<<<, =======, and
>>>>>>> lines while preserving the function and its comment, and verify no other
conflict markers remain in this area.
In `@pkg/planner/core/expression_rewriter.go`:
- Around line 380-387: Resolve the merge conflict in the expressionRewriter
struct by removing all conflict markers and keeping the incoming fields
astNodeStack and useNewCollate alongside planCtx, since expressionRewriter
methods such as the ones referencing er.useNewCollate depend on that field to
compile. Make sure the struct definition is consistent with the rest of
expressionRewriter in expression_rewriter.go and that no HEAD-only version of
the struct remains.
In `@pkg/table/tables/index.go`:
- Around line 32-57: Resolve the remaining merge conflict markers in the
package-level declarations and imports in index.go so the file can compile
again. Remove all raw conflict artifacts like the HEAD/ab1e19714d6 sections and
reconcile the duplicated imports and declarations around indexConditionECtx and
indexPartialCondition, keeping the intended definitions from the merged changes
in this file. Verify any affected symbols in the table/index logic still
reference the same types and helpers after the conflict cleanup.
- Line 46: The global index parsing context is left as a zero-value
BuildContext, which can panic when ParseSimpleExpr emits warnings during
partial-index condition parsing. Initialize indexConditionECtx with a valid
exprctx.BuildContext before it is used by the index condition parsing path in
tables/index.go, and make sure the ConditionExprString handling always passes a
non-nil evaluation context through ParseSimpleExpr and ctx.GetEvalCtx().
In `@pkg/table/tables/tables.go`:
- Around line 270-277: Resolve the merge conflict markers in initTableIndices by
removing the <<<<<<<, =======, and >>>>>>> text and keeping the
NewIndexWithCollate(t.encoder.UseNewCollate(), t.physicalTableID, tblInfo,
idxInfo) path with its err check and early return. Make sure the resolved code
in tables.go uses the NewIndexWithCollate call instead of NewIndex so the index
creation remains explicit about collation handling.
In `@pkg/table/tables/testutil/indexcheck.go`:
- Around line 38-48: The index lookup in the test helper leaves idx nil when
indexName is not found, and the later idx.Meta() call will panic instead of
reporting a useful test failure. In indexcheck.go, update the lookup around
tbl.Indices() to explicitly assert that a matching index was found before
calling tablecodec.GenIndexKey, using the existing require-based test flow in
this helper so the failure is actionable and tied to the indexName/table.Index
search.
In `@pkg/tablecodec/tablecodec_test.go`:
- Around line 751-752: The test file contains unresolved merge-conflict markers
that prevent compilation; remove the conflict markers from the affected test
sections and keep the incoming cherry-pick side’s additions. Update the
`tablecodec_test.go` test blocks around the new test functions so only the
intended test code remains, and also clean up the same merge-conflict marker
occurrence in the other referenced section.
- Around line 888-889: The test assertion in tablecodec_test.go is using
require.NotContains on key with a []byte value, which makes the check
ineffective. Update the unique index key assertion in the relevant test case to
compare against PartitionIDFlag directly, or switch to the same byte-scan style
used for the positive check below so the absence of the flag is actually
validated.
In `@pkg/tablecodec/tablecodec.go`:
- Around line 1621-1625: Resolve the merge conflict in the tablecodec
restoration check by removing the conflict markers and keeping the incoming
collation-aware path in the relevant tablecodec logic; update the condition that
currently references types.NeedRestoredData and use the version that calls
types.NeedRestoredDataWithCollate with model.GetIdxChangingFieldType(idxCol,
col) and useNewCollate so the collation flag is threaded through explicitly.
---
Nitpick comments:
In `@pkg/table/tblctx/buffers.go`:
- Around line 90-97: EncodeBinlogRowData still creates its own encoder with
collate.NewCollationEnabled(), which leaves an implicit global dependency and
can disagree with the caller’s collation context. Update EncodeBinlogRowData to
accept a codec.Encoder parameter, and thread that encoder through to
tablecodec.EncodeOldRow instead of constructing it locally, matching the
explicit encoder flow used by WriteMemBufferEncoded and the EncodeRowBuffer
type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ec4d1205-950f-4f14-b7c0-fc8d5782b3fb
📒 Files selected for processing (41)
pkg/ddl/column.gopkg/ddl/index.gopkg/executor/admin.gopkg/executor/test/executor/BUILD.bazelpkg/executor/test/executor/executor_test.gopkg/executor/write.gopkg/expression/expression.gopkg/meta/model/table.gopkg/planner/core/expression_rewriter.gopkg/server/handler/tests/BUILD.bazelpkg/server/handler/tests/http_handler_test.gopkg/session/BUILD.bazelpkg/session/global_init.gopkg/session/session.gopkg/store/mockstore/BUILD.bazelpkg/store/mockstore/cluster_test.gopkg/store/mockstore/unistore/cophandler/cop_handler_test.gopkg/table/tables/index.gopkg/table/tables/mutation_checker.gopkg/table/tables/mutation_checker_test.gopkg/table/tables/partition.gopkg/table/tables/tables.gopkg/table/tables/testutil/BUILD.bazelpkg/table/tables/testutil/indexcheck.gopkg/table/tblctx/BUILD.bazelpkg/table/tblctx/buffers.gopkg/table/tblctx/buffers_test.gopkg/tablecodec/tablecodec.gopkg/tablecodec/tablecodec_test.gopkg/testkit/mockstore.gopkg/types/etc.gopkg/util/codec/codec.gopkg/util/codec/codec_test.gopkg/util/codec/collation_test.gopkg/util/collate/collate.gopkg/util/rowDecoder/BUILD.bazelpkg/util/rowDecoder/decoder_test.gopkg/util/rowcodec/bench_test.gopkg/util/rowcodec/rowcodec_test.gotests/realtikvtest/addindextest2/BUILD.bazeltests/realtikvtest/addindextest2/global_sort_test.go
| <<<<<<< HEAD | ||
| ======= | ||
| // GetIdxChangingFieldType gets the field type of index column. | ||
| // Since both old/new type may coexist in one column during modify column, | ||
| // we need to get the correct type for index column. | ||
| func GetIdxChangingFieldType(idxCol *IndexColumn, col *ColumnInfo) *types.FieldType { | ||
| if idxCol.UseChangingType && col.ChangingFieldType != nil { | ||
| return col.ChangingFieldType | ||
| } | ||
| return &col.FieldType | ||
| } | ||
|
|
||
| >>>>>>> ab1e19714d6 (codec, table: make new collation setting explicit in encoding (#69566)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unresolved merge conflict — package will not compile.
Lines 584-596 still contain Git conflict markers (<<<<<<< HEAD, =======, >>>>>>>), which static analysis flags as syntax errors. Resolve by dropping the markers and keeping the incoming GetIdxChangingFieldType helper.
🐛 Proposed resolution
-<<<<<<< HEAD
-=======
// GetIdxChangingFieldType gets the field type of index column.
// Since both old/new type may coexist in one column during modify column,
// we need to get the correct type for index column.
func GetIdxChangingFieldType(idxCol *IndexColumn, col *ColumnInfo) *types.FieldType {
if idxCol.UseChangingType && col.ChangingFieldType != nil {
return col.ChangingFieldType
}
return &col.FieldType
}
-
->>>>>>> ab1e19714d6 (codec, table: make new collation setting explicit in encoding (`#69566`))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <<<<<<< HEAD | |
| ======= | |
| // GetIdxChangingFieldType gets the field type of index column. | |
| // Since both old/new type may coexist in one column during modify column, | |
| // we need to get the correct type for index column. | |
| func GetIdxChangingFieldType(idxCol *IndexColumn, col *ColumnInfo) *types.FieldType { | |
| if idxCol.UseChangingType && col.ChangingFieldType != nil { | |
| return col.ChangingFieldType | |
| } | |
| return &col.FieldType | |
| } | |
| >>>>>>> ab1e19714d6 (codec, table: make new collation setting explicit in encoding (#69566)) | |
| // GetIdxChangingFieldType gets the field type of index column. | |
| // Since both old/new type may coexist in one column during modify column, | |
| // we need to get the correct type for index column. | |
| func GetIdxChangingFieldType(idxCol *IndexColumn, col *ColumnInfo) *types.FieldType { | |
| if idxCol.UseChangingType && col.ChangingFieldType != nil { | |
| return col.ChangingFieldType | |
| } | |
| return &col.FieldType | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 584-584: : # github.com/pingcap/tidb/pkg/meta/model [github.com/pingcap/tidb/pkg/meta/model.test]
pkg/meta/model/table.go:584:1: syntax error: non-declaration statement outside function body
pkg/meta/model/table.go:596:1: syntax error: non-declaration statement outside function body
pkg/meta/model/table.go:596:85: invalid character U+0023 '#'
(typecheck)
[error] 584-584: expected declaration, found '<<'
(typecheck)
[error] 596-596: illegal character U+0023 '#'
(typecheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/meta/model/table.go` around lines 584 - 596, Resolve the merge conflict
in table.go by removing the Git conflict markers and keeping the incoming
GetIdxChangingFieldType helper. Ensure the table package compiles by deleting
the leftover <<<<<<<, =======, and >>>>>>> lines while preserving the function
and its comment, and verify no other conflict markers remain in this area.
Source: Linters/SAST tools
| <<<<<<< HEAD | ||
| planCtx *exprRewriterPlanCtx | ||
| ======= | ||
| astNodeStack []ast.Node | ||
|
|
||
| planCtx *exprRewriterPlanCtx | ||
| useNewCollate bool | ||
| >>>>>>> ab1e19714d6 (codec, table: make new collation setting explicit in encoding (#69566)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unresolved merge conflict in expressionRewriter struct — won't compile.
The conflict markers must be removed. Note the HEAD side omits useNewCollate (and astNodeStack), yet er.useNewCollate is referenced at Line 155, Line 265, Line 1666, Line 2082, and Line 2186. Keep the incoming side so those references resolve.
🐛 Proposed resolution
-<<<<<<< HEAD
- planCtx *exprRewriterPlanCtx
-=======
astNodeStack []ast.Node
planCtx *exprRewriterPlanCtx
useNewCollate bool
->>>>>>> ab1e19714d6 (codec, table: make new collation setting explicit in encoding (`#69566`))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <<<<<<< HEAD | |
| planCtx *exprRewriterPlanCtx | |
| ======= | |
| astNodeStack []ast.Node | |
| planCtx *exprRewriterPlanCtx | |
| useNewCollate bool | |
| >>>>>>> ab1e19714d6 (codec, table: make new collation setting explicit in encoding (#69566)) | |
| astNodeStack []ast.Node | |
| planCtx *exprRewriterPlanCtx | |
| useNewCollate bool |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/expression_rewriter.go` around lines 380 - 387, Resolve the
merge conflict in the expressionRewriter struct by removing all conflict markers
and keeping the incoming fields astNodeStack and useNewCollate alongside
planCtx, since expressionRewriter methods such as the ones referencing
er.useNewCollate depend on that field to compile. Make sure the struct
definition is consistent with the rest of expressionRewriter in
expression_rewriter.go and that no HEAD-only version of the struct remains.
| <<<<<<< HEAD | ||
| idx := NewIndex(t.physicalTableID, tblInfo, idxInfo) | ||
| ======= | ||
| idx, err := NewIndexWithCollate(t.encoder.UseNewCollate(), t.physicalTableID, tblInfo, idxInfo) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| >>>>>>> ab1e19714d6 (codec, table: make new collation setting explicit in encoding (#69566)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unresolved merge conflict markers in initTableIndices.
Lines 270-277 still contain <<<<<<< HEAD / ======= / >>>>>>> markers. This won't compile until resolved to the NewIndexWithCollate(...) branch (with its error handling).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/table/tables/tables.go` around lines 270 - 277, Resolve the merge
conflict markers in initTableIndices by removing the <<<<<<<, =======, and
>>>>>>> text and keeping the NewIndexWithCollate(t.encoder.UseNewCollate(),
t.physicalTableID, tblInfo, idxInfo) path with its err check and early return.
Make sure the resolved code in tables.go uses the NewIndexWithCollate call
instead of NewIndex so the index creation remains explicit about collation
handling.
| var idx table.Index | ||
| for _, i := range tbl.Indices() { | ||
| if i.Meta().Name.L == indexName { | ||
| idx = i | ||
| break | ||
| } | ||
| } | ||
|
|
||
| enc := codec.NewEncoder(collate.NewCollationEnabled()) | ||
| minimumKey, _, err := tablecodec.GenIndexKey(enc, time.Local, tbl.Meta(), idx.Meta(), tbl.Meta().ID, nil, nil, nil) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Nil idx will panic if indexName doesn't match any index.
If no index in tbl.Indices() matches indexName, idx remains its zero value (nil table.Index interface). The subsequent idx.Meta() call at Line 47 then panics with a nil-interface dereference instead of failing with an actionable test error.
🛡️ Proposed fix
var idx table.Index
for _, i := range tbl.Indices() {
if i.Meta().Name.L == indexName {
idx = i
break
}
}
+ require.NotNil(t, idx, "index %q not found on table %q", indexName, tableName)
enc := codec.NewEncoder(collate.NewCollationEnabled())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var idx table.Index | |
| for _, i := range tbl.Indices() { | |
| if i.Meta().Name.L == indexName { | |
| idx = i | |
| break | |
| } | |
| } | |
| enc := codec.NewEncoder(collate.NewCollationEnabled()) | |
| minimumKey, _, err := tablecodec.GenIndexKey(enc, time.Local, tbl.Meta(), idx.Meta(), tbl.Meta().ID, nil, nil, nil) | |
| require.NoError(t, err) | |
| var idx table.Index | |
| for _, i := range tbl.Indices() { | |
| if i.Meta().Name.L == indexName { | |
| idx = i | |
| break | |
| } | |
| } | |
| require.NotNil(t, idx, "index %q not found on table %q", indexName, tableName) | |
| enc := codec.NewEncoder(collate.NewCollationEnabled()) | |
| minimumKey, _, err := tablecodec.GenIndexKey(enc, time.Local, tbl.Meta(), idx.Meta(), tbl.Meta().ID, nil, nil, nil) | |
| require.NoError(t, err) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/table/tables/testutil/indexcheck.go` around lines 38 - 48, The index
lookup in the test helper leaves idx nil when indexName is not found, and the
later idx.Meta() call will panic instead of reporting a useful test failure. In
indexcheck.go, update the lookup around tbl.Indices() to explicitly assert that
a matching index was found before calling tablecodec.GenIndexKey, using the
existing require-based test flow in this helper so the failure is actionable and
tied to the indexName/table.Index search.
| <<<<<<< HEAD | ||
| ======= |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unresolved merge-conflict markers — test file will not compile.
Remove the <<<<<<< HEAD / ======= / >>>>>>> markers, keeping the two new test functions from the incoming (cherry-pick) side.
Also applies to: 980-980
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/tablecodec/tablecodec_test.go` around lines 751 - 752, The test file
contains unresolved merge-conflict markers that prevent compilation; remove the
conflict markers from the affected test sections and keep the incoming
cherry-pick side’s additions. Update the `tablecodec_test.go` test blocks around
the new test functions so only the intended test code remains, and also clean up
the same merge-conflict marker occurrence in the other referenced section.
| require.NotContains(t, key, []byte{PartitionIDFlag}, | ||
| "unique index key with non-NULL value should NOT contain partition ID") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant region around the reported lines.
sed -n '860,930p' pkg/tablecodec/tablecodec_test.go
# Search for similar assertions in the file for context.
rg -n 'NotContains|Contains' pkg/tablecodec/tablecodec_test.goRepository: pingcap/tidb
Length of output: 2991
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Identify testify version and available module cache paths.
rg -n 'testify' go.mod go.sum || true
go env GOPATH GOMODCACHE
# Locate testify's contain helper source in the module cache if present.
python3 - <<'PY'
import os, glob
gopath = os.popen('go env GOPATH').read().strip()
gomodcache = os.popen('go env GOMODCACHE').read().strip()
candidates = []
for base in [gomodcache, os.path.join(gopath, 'pkg', 'mod')]:
if base and os.path.isdir(base):
candidates.extend(glob.glob(os.path.join(base, 'github.com', 'stretchr', 'testify*')))
print('\n'.join(sorted(set(candidates))))
PY
# Search for NotContains implementation if testify is cached.
for d in "$(go env GOMODCACHE)" "$(go env GOPATH)/pkg/mod"; do
if [ -d "$d" ]; then
rg -n 'func NotContains|Contains(' "$d"/github.com/stretchr/testify* 2>/dev/null | head -n 50 || true
fi
doneRepository: pingcap/tidb
Length of output: 1849
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TESTIFY_DIR="$(go env GOMODCACHE)/github.com/stretchr/testify@v1.11.1"
# Show the relevant implementation.
rg -n 'func (Contains|NotContains)|case reflect.Slice|bytes\.Contains|slices\.Contains|deepEqual' "$TESTIFY_DIR/assert" "$TESTIFY_DIR/require" || true
sed -n '1,260p' "$TESTIFY_DIR/assert/assertions.go"
# Run a tiny probe to emulate the path the helper would take for []byte values.
cat > /tmp/testify_probe.go <<'EOF'
package main
import (
"bytes"
"fmt"
"reflect"
)
func main() {
key := []byte{1, 2, 3}
needleSlice := []byte{1}
needleByte := byte(1)
fmt.Println("bytes.Contains(key, needleSlice):", bytes.Contains(key, needleSlice))
fmt.Println("reflect.DeepEqual(key[0], needleSlice):", reflect.DeepEqual(key[0], needleSlice))
fmt.Println("reflect.DeepEqual(key[0], needleByte):", reflect.DeepEqual(key[0], needleByte))
}
EOF
go run /tmp/testify_probe.goRepository: pingcap/tidb
Length of output: 9514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TESTIFY_DIR="$(go env GOMODCACHE)/github.com/stretchr/testify@v1.11.1"
# Show the relevant implementation.
rg -n 'func (Contains|NotContains)|case reflect.Slice|bytes\.Contains|slices\.Contains|deepEqual' "$TESTIFY_DIR/assert" "$TESTIFY_DIR/require" || true
sed -n '1,260p' "$TESTIFY_DIR/assert/assertions.go"
# Run a tiny probe to inspect byte-slice containment semantics.
cat > /tmp/testify_probe.go <<'EOF'
package main
import (
"bytes"
"fmt"
"reflect"
)
func main() {
key := []byte{1, 2, 3}
needleSlice := []byte{1}
needleByte := byte(1)
fmt.Println("bytes.Contains(key, needleSlice):", bytes.Contains(key, needleSlice))
fmt.Println("reflect.DeepEqual(key[0], needleSlice):", reflect.DeepEqual(key[0], needleSlice))
fmt.Println("reflect.DeepEqual(key[0], needleByte):", reflect.DeepEqual(key[0], needleByte))
}
EOF
go run /tmp/testify_probe.goRepository: pingcap/tidb
Length of output: 9514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TESTIFY_DIR="$(go env GOMODCACHE)/github.com/stretchr/testify@v1.11.1"
# Show the exact implementation of Contains / NotContains.
sed -n '950,1015p' "$TESTIFY_DIR/assert/assertions.go"
# Exercise the actual helper with a fake TestingT.
tmpdir="$(mktemp -d)"
cat > "$tmpdir/go.mod" <<'EOF'
module probe
go 1.23
require github.com/stretchr/testify v1.11.1
EOF
cat > "$tmpdir/main.go" <<'EOF'
package main
import (
"fmt"
"github.com/stretchr/testify/assert"
)
type fakeT struct{ failed bool }
func (t *fakeT) Errorf(format string, args ...interface{}) {
t.failed = true
fmt.Printf("Errorf: "+format+"\n", args...)
}
func main() {
var t fakeT
fmt.Println("case1", assert.NotContains(&t, []byte{1, 2, 3}, []byte{1}))
fmt.Println("case2_failed", t.failed)
t = fakeT{}
fmt.Println("case2", assert.NotContains(&t, []byte{1, 2, 3}, byte(1)))
fmt.Println("case2_failed", t.failed)
t = fakeT{}
fmt.Println("case3", assert.NotContains(&t, []byte{1, 2, 3}, []byte{4}))
fmt.Println("case3_failed", t.failed)
}
EOF
( cd "$tmpdir" && GONOSUMDB='*' GOPROXY=off go run . )Repository: pingcap/tidb
Length of output: 2375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TESTIFY_DIR="$(go env GOMODCACHE)/github.com/stretchr/testify@v1.11.1"
rg -n 'func containsElement|containsElement\(' "$TESTIFY_DIR/assert/assertions.go"
sed -n '280,380p' "$TESTIFY_DIR/assert/assertions.go"Repository: pingcap/tidb
Length of output: 3283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TESTIFY_DIR="$(go env GOMODCACHE)/github.com/stretchr/testify@v1.11.1"
sed -n '917,960p' "$TESTIFY_DIR/assert/assertions.go"Repository: pingcap/tidb
Length of output: 1348
Fix the NotContains check on key
require.NotContains(t, key, []byte{PartitionIDFlag}) compares each byte in key against a []byte, so it can never fail. Use PartitionIDFlag here, or keep the byte scan used below for the positive case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/tablecodec/tablecodec_test.go` around lines 888 - 889, The test assertion
in tablecodec_test.go is using require.NotContains on key with a []byte value,
which makes the check ineffective. Update the unique index key assertion in the
relevant test case to compare against PartitionIDFlag directly, or switch to the
same byte-scan style used for the positive check below so the absence of the
flag is actually validated.
| <<<<<<< HEAD | ||
| if types.NeedRestoredData(&col.FieldType) { | ||
| ======= | ||
| if types.NeedRestoredDataWithCollate(model.GetIdxChangingFieldType(idxCol, col), useNewCollate) { | ||
| >>>>>>> ab1e19714d6 (codec, table: make new collation setting explicit in encoding (#69566)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unresolved merge-conflict markers — file will not compile.
The <<<<<<< HEAD / ======= / >>>>>>> markers are still present. Resolve by keeping the cherry-picked (incoming) branch so the collation flag is threaded through, and drop the HEAD variant that reads the global setting implicitly.
🐛 Proposed resolution
-<<<<<<< HEAD
- if types.NeedRestoredData(&col.FieldType) {
-=======
- if types.NeedRestoredDataWithCollate(model.GetIdxChangingFieldType(idxCol, col), useNewCollate) {
->>>>>>> ab1e19714d6 (codec, table: make new collation setting explicit in encoding (`#69566`))
+ if types.NeedRestoredDataWithCollate(model.GetIdxChangingFieldType(idxCol, col), useNewCollate) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <<<<<<< HEAD | |
| if types.NeedRestoredData(&col.FieldType) { | |
| ======= | |
| if types.NeedRestoredDataWithCollate(model.GetIdxChangingFieldType(idxCol, col), useNewCollate) { | |
| >>>>>>> ab1e19714d6 (codec, table: make new collation setting explicit in encoding (#69566)) | |
| if types.NeedRestoredDataWithCollate(model.GetIdxChangingFieldType(idxCol, col), useNewCollate) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/tablecodec/tablecodec.go` around lines 1621 - 1625, Resolve the merge
conflict in the tablecodec restoration check by removing the conflict markers
and keeping the incoming collation-aware path in the relevant tablecodec logic;
update the condition that currently references types.NeedRestoredData and use
the version that calls types.NeedRestoredDataWithCollate with
model.GetIdxChangingFieldType(idxCol, col) and useNewCollate so the collation
flag is threaded through explicitly.
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: D3Hunter The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/infoschema/issyncer/loader.go (1)
293-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew filter-driven paths lack direct unit tests and a brief intent comment.
skipLoadingDiff's filter check and the newIterDatabases+SkipLoadSchemabranch are both new critical decision paths, butloader_test.goonly updates constructors to passnilforfilter— no test exercisesSkipLoadDiff/SkipLoadSchemareturningtrue. Also, unlike thecrossKSbranch above it, the newelse if l.filter != nilbranch at Line 427 has no comment explaining its purpose (used by the lightweight bootstrap domain to load only selected schemas).Consider adding a fake
Filterinloader_test.goto cover bothskipLoadingDiffandfetchAllSchemasWithTablesfiltering behavior, and a one-line comment above Line 427.Also applies to: 427-437
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/infoschema/issyncer/loader.go` around lines 293 - 303, Add unit coverage for the new filter-driven decision paths in Loader: create a fake Filter in loader_test.go that exercises skipLoadingDiff when SkipLoadDiff returns true and fetchAllSchemasWithTables when IterDatabases/SkipLoadSchema causes schemas to be skipped. Also add a brief intent comment above the new else if l.filter != nil branch in Loader to explain it is used by the lightweight bootstrap domain to load only selected schemas.Source: Coding guidelines
pkg/meta/reader.go (1)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the function parameter for consistency with
IterTables.
IterTables(dbID int64, fn func(info *model.TableInfo) error) errornames its callbackfn; the newIterDatabasesleaves its callback unnamed, which is inconsistent style within the same interface.As per coding guidelines, "Go code: Follow existing package-local conventions first and keep style consistent with nearby files."
✏️ Proposed fix
- IterDatabases(func(info *model.DBInfo) error) error + IterDatabases(fn func(info *model.DBInfo) error) error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/meta/reader.go` at line 32, The IterDatabases method in the reader interface leaves its callback parameter unnamed, which is inconsistent with IterTables. Update the IterDatabases signature to name the func argument using the same local convention as IterTables (for example, fn) so the interface stays stylistically consistent with nearby code.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/infoschema/issyncer/loader.go`:
- Around line 293-303: Add unit coverage for the new filter-driven decision
paths in Loader: create a fake Filter in loader_test.go that exercises
skipLoadingDiff when SkipLoadDiff returns true and fetchAllSchemasWithTables
when IterDatabases/SkipLoadSchema causes schemas to be skipped. Also add a brief
intent comment above the new else if l.filter != nil branch in Loader to explain
it is used by the lightweight bootstrap domain to load only selected schemas.
In `@pkg/meta/reader.go`:
- Line 32: The IterDatabases method in the reader interface leaves its callback
parameter unnamed, which is inconsistent with IterTables. Update the
IterDatabases signature to name the func argument using the same local
convention as IterTables (for example, fn) so the interface stays stylistically
consistent with nearby code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5f015ae1-2616-4c5b-ba8b-7b455d9e9595
📒 Files selected for processing (19)
pkg/domain/domain.gopkg/infoschema/issyncer/BUILD.bazelpkg/infoschema/issyncer/filter.gopkg/infoschema/issyncer/loader.gopkg/infoschema/issyncer/loader_test.gopkg/infoschema/issyncer/syncer.gopkg/infoschema/issyncer/syncer_test.gopkg/meta/meta.gopkg/meta/reader.gopkg/planner/core/expression_rewriter.gopkg/session/BUILD.bazelpkg/session/bootstrap_test.gopkg/session/tidb.gopkg/store/copr/copr_test/BUILD.bazelpkg/table/tables/index.gopkg/table/tables/mutation_checker_test.gopkg/table/tables/tables.gopkg/tablecodec/tablecodec.gopkg/tablecodec/tablecodec_test.go
💤 Files with no reviewable changes (2)
- pkg/tablecodec/tablecodec_test.go
- pkg/planner/core/expression_rewriter.go
✅ Files skipped from review due to trivial changes (1)
- pkg/store/copr/copr_test/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/session/BUILD.bazel
- pkg/table/tables/tables.go
- pkg/table/tables/mutation_checker_test.go
- pkg/tablecodec/tablecodec.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-nextgen-20251011 #69684 +/- ##
=============================================================
Coverage ? 71.8435%
=============================================================
Files ? 1836
Lines ? 494099
Branches ? 0
=============================================================
Hits ? 354978
Misses ? 115713
Partials ? 23408
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This is an automated cherry-pick of #69566
What problem does this PR solve?
Issue Number: ref #69563
Problem Summary:
Some lower-level codec and tablecodec helper paths read the global new-collation setting directly when deciding how to encode values or whether restored data is needed. That makes deep utility behavior depend on process-wide state and makes the new-collation global harder to eliminate incrementally.
This PR is the first part of the refactor. It removes the implicit global dependency from the encoding paths touched here by threading an explicit collation setting through the relevant table, tablecodec, and codec helpers. Follow-up work should continue eliminating the remaining global variable usage.
in the first phase, we we will only refactor this part to make sure nextgen addindex/import-into can work in the case in the issue
What changed and how does it work?
codec.Encoder, which stores the new-collation setting used byEncodeKey,EncodeValue, andHashCode.codec.Encoderthrough row and index encoding helpers intablecodec,table/tables, andtable/tblctx.NeedRestoredDataWithCollatewhen the collation setting is already known.Check List
Tests
Unit tests:
./tools/check/failpoint-go-test.sh pkg/util/codec -run TestEncoderNewCollationEnabled -count=1./tools/check/failpoint-go-test.sh pkg/tablecodec -run 'Test(RowCodec|DecodeColumnValue|TimeCodec|CutRow|UniqueGlobalIndexKeyWithNullValues)$' -count=1./tools/check/failpoint-go-test.sh pkg/table/tables -run 'Test(CheckRowInsertionConsistency|CheckIndexKeysAndCheckHandleConsistency)$' -count=1pushd pkg/table/tblctx >/dev/null && go test -run 'Test(EncodeRow|EncodeBufferReserve)$' -tags=intest,deadlock -count=1 && popd >/dev/nullManual validation:
make bazel_preparegit diff --check master...HEADmake lintSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Bug Fixes